Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds TZ info to all CFP start/end/announce dates #5213

Merged
merged 4 commits into from
Sep 1, 2018

Conversation

tgross
Copy link
Contributor

@tgross tgross commented Aug 25, 2018

@tgross
Copy link
Contributor Author

tgross commented Aug 25, 2018

Currently a merge conflict b/c I need to update a few dates that got changed since I started the PR. Fix coming shortly.

@tgross
Copy link
Contributor Author

tgross commented Aug 25, 2018

Merged conflict fixed.

@tgross
Copy link
Contributor Author

tgross commented Aug 25, 2018

I was just re-testing this locally and while this solves devopsdays/devopsdays-theme#476 it doesn't solve https://github.com/devopsdays/devopsdays-theme/issues/487 like I hoped. That appears to be entirely client-side where we have just the 2018-08-23 strings and nothing to do with our timestamp handling.

@tgross
Copy link
Contributor Author

tgross commented Aug 25, 2018

cc @bridgetkromhout

I have another half-finished commit waiting in the wings to add this data to start/end dates and registration dates, but I want to verify that we really want to do that for those before I wrap it up.

@bridgetkromhout
Copy link
Collaborator

bridgetkromhout commented Aug 25, 2018

I want to verify that we really want to do that for those

@tgross, thanks! indeed, I'm not sure about that.

I do have a possibly-confusing "what even is timezones" question, though - specifically, when an event says their CFP "closes" on a specific "day", I think the common interpretation is "23:59:59 in that timezone". That is to say, if a CFP closes on 2018-01-01, you have all day long on the 1st of January to get your submission in. If I am understanding time correctly (not entirely a given!), I think what we're seeing here in this very welcome and helpful PR is "close the CFP the instant it becomes January 1st!" which is probably not a desirable default, as it may confuse the organizers who are thinking of the day as being "end of day" and not noticing the change. Thoughts?

@tgross
Copy link
Contributor Author

tgross commented Aug 25, 2018

Yeah from a user experience that's definitely something I hadn't considered. We could change the end times to be 23:59?

Also, how does that actually work given this is a static site? Is it getting periodically re-rendered on a timer?

@bridgetkromhout
Copy link
Collaborator

Yeah from a user experience that's definitely something I hadn't considered. We could change the end times to be 23:59?

I think that would probably be wise. Let's assume that anyone who wants their CFP to close at 7pm or whatever would change it from the default, but I think that in the absence of clarifying details, people proposing talks would assume 23:59 conference-location-local time.

Also, how does that actually work given this is a static site? Is it getting periodically re-rendered on a timer?

The devopsdays.org site gets built and pushed to production whenever we merge a pull request. There are a few elements that will update more frequently (the one that comes to mind is the twitter feed box on the front page and on some event sites), but yes, the site isn't date-aware to the point that it would rebuild based on the date itself alone. However, if you look at the commit history, and consider that we have 70 active events for 2018, you can see that we probably don't need to spend cycles adding a "build if it hasn't built for a while". 😀

@tgross
Copy link
Contributor Author

tgross commented Aug 27, 2018

Let's assume that anyone who wants their CFP to close at 7pm or whatever would change it from the default, but I think that in the absence of clarifying details, people proposing talks would assume 23:59 conference-location-local time.

One quick sed later and that's done.

@bridgetkromhout
Copy link
Collaborator

I want to verify that we really want to do that for those

I think we came down at "yes" for this (when we do the next theme release).

Looking at this work-in-progress I see some cities missing from your "let's update all the 2018/2019 events"? You can see this if you check out the /speaking page on a local build of this branch or on the netlify preview: https://deploy-preview-5213--devopsdays-web.netlify.com/speaking/

screen shot 2018-08-27 at 2 57 55 pm

So, before we merge this I think we may want to make sure they're all up to date (I realize this is a moving target!)

In terms of timing for merging this, we also may want to wait until after the code release that supports this, because the current codebase just shows the entire string which will look a little confusing.

@tgross
Copy link
Contributor Author

tgross commented Aug 28, 2018

So, before we merge this I think we may want to make sure they're all up to date (I realize this is a moving target!)
In terms of timing for merging this, we also may want to wait until after the code release that supports this, because the current codebase just shows the entire string which will look a little confusing.

Yeah, just to avoid a bunch of rework I'll hold off on making that data fix until the theme update gets deployed. But once that's been done I'll knock that out.

@mattstratton
Copy link
Member

Just want to confirm that we can roll out the theme changes you've done so far without this PR?

@tgross
Copy link
Contributor Author

tgross commented Aug 31, 2018

Yes. 👍

@bridgetkromhout
Copy link
Collaborator

Tested this by testing various cfp closure datetimes for one of the events and watching it appear/disappear from the speaking page. Also checked to make sure the {{< cfp_dates >}} entry on a propose page still displays correctly. Looks great!

@bridgetkromhout
Copy link
Collaborator

Yeah, just to avoid a bunch of rework I'll hold off on making that data fix until the theme update gets deployed. But once that's been done I'll knock that out.

Thanks. I'm going to merge this PR because it will work now and there's no reason to leave it lingering. Happy to take more such data-fixing PRs now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants